Skip to content

fix: emit absolute -fprebuilt-module-path so clangd resolves modules#77

Merged
Sunrisepeak merged 4 commits into
mainfrom
fix/cdb-prebuilt-module-path-abs
May 24, 2026
Merged

fix: emit absolute -fprebuilt-module-path so clangd resolves modules#77
Sunrisepeak merged 4 commits into
mainfrom
fix/cdb-prebuilt-module-path-abs

Conversation

@Sunrisepeak
Copy link
Copy Markdown
Member

Problem

mcpp emits -fprebuilt-module-path=pcm.cache (bare relative) in the compile command. ninja runs with cwd = <projectRoot>/target/<triple>/<fp>/ so the bare relative path resolves to the actual BMI cache and the build succeeds. But the same flag is captured verbatim into compile_commands.json, whose directory field is set to plan.projectRoot.

Per the CDB spec, clangd does cd <directory> before resolving the arguments. So clangd looks for <projectRoot>/pcm.cache — which doesn't exist. The user-visible symptom is import resolution silently failing in the editor:

module 'mcpplibs.cmdline' not found
All checks completed, 2 errors

… even though mcpp build itself works perfectly.

Root cause

src/build/flags.cppm:215 before:

prebuilt_module_flag = std::format(" -fprebuilt-module-path={}", traits.bmiDir);
//                                                              ^ bare "pcm.cache" / "gcm.cache"

traits.bmiDir is just "pcm.cache" (Clang) or "gcm.cache" (GCC). All the other -fmodule-file= flags in the same block are already wrapped in escape_path(<absolute_path>). This one was a leftover.

Fix

prebuilt_module_flag = std::format(" -fprebuilt-module-path={}",
    escape_path(plan.outputDir / traits.bmiDir));

Single line. ninja still works (absolute paths are cwd-independent). clangd now finds the BMI cache.

Test

New tests/e2e/47_cdb_prebuilt_module_path_abs.sh:

  • mcpp new app && mcpp build
  • Parse compile_commands.json, extract every -fprebuilt-module-path=X
  • Assert each X:
    • is absolute (starts with / on POSIX or <drive>: on Windows)
    • ends in /pcm.cache or /gcm.cache
    • points to an existing directory
  • Pass-through when the flag isn't emitted (GCC -fmodules-only flow)

Test plan

  • ci-linux (incl. new e2e 47, plus existing 02/03/multi-module tests)
  • ci-macos
  • ci-windows

Risk

Minimal. The flag is functionally identical (absolute path resolves the same as the cwd-relative path did, since ninja's cwd is exactly <outputDir>). The change only affects how the flag looks in the captured command string.

src/build/flags.cppm was emitting a bare `-fprebuilt-module-path=pcm.cache`
(or `=gcm.cache`). This works for `mcpp build` because ninja runs commands
with cwd = `<projectRoot>/target/<triple>/<fp>/` so the relative path
resolves correctly. But the same flag is captured verbatim into
`compile_commands.json`, whose `directory` field is `plan.projectRoot`.
Per CDB spec, clangd does `cd <directory>` before resolving the args, so
it looks for `<projectRoot>/pcm.cache` — which doesn't exist. Effect:
`import` resolution silently fails in clangd ("module 'X' not found"),
even when `mcpp build` succeeds.

The other `-fmodule-file=` flags in the same block were already absolute
(escape_path-wrapped); this one was an oversight.

Fix: format the flag with `escape_path(plan.outputDir / traits.bmiDir)`.
Single line. ninja still works (absolute paths are cwd-independent), and
clangd now finds the BMI cache.

Regression test: tests/e2e/47_cdb_prebuilt_module_path_abs.sh creates a
fresh project, runs `mcpp build`, parses `compile_commands.json`, asserts
every `-fprebuilt-module-path=X`:
  - is absolute (starts with `/` on POSIX or `<drive>:` on Windows)
  - ends in `/pcm.cache` or `/gcm.cache`
  - points to an existing directory
Pass-through for GCC-only flows that don't emit the flag at all.
First CI round revealed two layered issues exposed by the new e2e:

1. **The shipped CDB carries ninja-escape artefacts.** `flags.cppm`'s
   `escape_path` ninja-escapes paths (`$ ` for space, `$:` for colon,
   `$$` for literal `$`) so they survive embedding in ninja rule strings.
   But the same escaped strings flow verbatim through `f.cxx` into
   `compile_commands.json`, whose `arguments` array is exec'd by clangd
   without any ninja involved. On Windows that makes `C:` appear as
   `C$:` in CDB → clangd can't resolve the path. POSIX usually escaped
   nothing in practice, hiding the bug; Windows drive letters always
   trigger `$:`.

   Fix in `compile_commands.cppm::split_flags`: same single pass now
   splits tokens *and* unescapes the three ninja sequences. Splitting
   was also broken for paths with a literal space (would've broken
   `-isystem "/path with space/include"` mid-arg) — that's fixed
   simultaneously since `$ ` is no longer a token separator.

2. **e2e test 47 needed Windows-friendly checks.** The original `[^"]+`
   grep returned the raw JSON-escaped form (`\\Users\\...`) and the
   `^[A-Za-z]:` regex didn't match `C$:`. Rewrote using `jq -r` (which
   does JSON un-escape natively, preinstalled on all GHA runners) and:
   - explicit ninja-escape sniff (`$:`, `$ `, `$$`)
   - both POSIX and Windows-drive absolute-path checks
   - basename check normalised against `\` / `/`

Net: CDB now contains plain paths in both runtimes, and the test fails
loudly if either layer regresses.
The Windows CI run reported:
  FAIL: basename is not pcm.cache/gcm.cache: 'pcm.cache
  '
The closing quote on the next line is the giveaway — the value carries a
trailing `\r`. `jq.exe` on git-bash emits CRLF; `mapfile -t` strips LF
but leaves CR, so every downstream comparison saw `pcm.cache$'\r'` and
failed. Trim with `${v%$'\r'}` per element.

The source-code fix (un-escape ninja sequences) is working — the value
itself is now a clean `C:\Users\...\pcm.cache`, no `$:` artefact.
Apple ships GPLv2-era bash 3.2 on macOS, which has no `mapfile` /
`readarray`. The previous round failed on ci-macos with:
  line 31: mapfile: command not found
  FAIL: 47_cdb_prebuilt_module_path_abs.sh (exit 127)

Replace `mapfile -t vals < <(jq …)` with `jq … > tmp.txt` followed by a
`while … read … done < tmp.txt`. Using input redirection (not a `|`
pipeline) keeps the loop in the current shell so `fail=1` propagates;
the temp file lives under $TMP and is cleaned up by the script's trap.

Empty-list signal now uses `[[ ! -s "$TMP/vals.txt" ]]` instead of array
length — same semantics, no bashism.
@Sunrisepeak Sunrisepeak merged commit b3627f9 into main May 24, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant